-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(protocol-designer): refactor well order modal to remove single edit dependencies #7375
Conversation
…le edit dependencies refactor well order modal to remove single edit dependencies
updateFirstWellOrder: $PropertyType<FieldProps, 'updateValue'>, | ||
updateSecondWellOrder: $PropertyType<FieldProps, 'updateValue'>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had to do this because this component needs two updaters (first well order and second).
class WellOrderModalComponent extends React.Component<Props, State> { | ||
export class WellOrderModal extends React.Component<Props, State> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about refactoring this to be a function component and use hooks but got lazy. I'm down to do that though if we think it's important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah we have enough to do for this project 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⏫
transfer and mix well orders behave the same as in prod. was able to import to prod to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functionality preserved in transfer and mix, code looks good. Annoying how in this one place, multiple fields get updated together, I just played around with the form and I didn't find any other where we're updating 2 keys together.
In theory, we will now enter a weird condition where updating the first well order field before the second gives us a nonsense well order, eg "l2r, l2r". But that's only in unsavedForm, so we should be safe. If it gives us problems we can think about special machinery for fields that need to update multiple values, but since this is the only one like that it seems a little extra RN
isOpen: boolean, | ||
closeModal: () => mixed, | ||
prefix: 'aspirate' | 'dispense' | 'mix', | ||
formData: FormData, | ||
updateValues: (firstValue: ?WellOrderOption, ?WellOrderOption) => void, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be like firstValue: ?WellOrderOption, secondValue: ?WellOrderOption
? I didn't know syntax would allow you to do just one named arg. Just a readability thing though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol i didn't even realize i did that — good catch will fix this real quick
class WellOrderModalComponent extends React.Component<Props, State> { | ||
export class WellOrderModal extends React.Component<Props, State> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah we have enough to do for this project 😛
Overview
Closes #7228
Changelog
Review requests
Risk assessment
Low